Code Review
手戻りを減らす工夫
金言が詰まってるmrsekut.icon
@shokai.iconのPRが多すぎるので、@shokai.iconのPRのみは、リリース後にコードレビューする会を実施してる
数が多くてレビューが大変
詳しくない人がレビューし
ても品質が上がらない
コードの土地勘は、結局レビューだけしてても得られない（と思う@shokai.icon） これ読みてえmrsekut.icon
対ビジネスユーザーの問い合わせ
一度にレビューできるコード量の限界
一度にレビューする量が400行を超えると、欠陥を見つける能力が低下する
この辺の知見をまとめた書籍とかないのかな？
自動化できるところは自動化する
CIで、testや型エラー、lintなどの検証を行う
レビューする時に考えるべきこと
どれぐらいの時間をかけるべきか
どういう手順でコードを見ていくべきか
commitを追っていく
diffだけみる、全部見る
コードだけ見る、実際に動かす
段階が考えられそう？mrsekut.icon
まず型エラー・テストで確認
この辺はCIにやらせるべき
まず動かしてみてちゃんと動くか確認
プログラム内部を確認
↑上にあるほど、自明なミスなので早い段階で却下できる(?)
指摘の仕方、伝え方
「俺が修正したほうが速い」という場合にどうするか
レビューの観点
段階というか、分類がありそうmrsekut.icon
プログラムの変更容易姓の諸々
セキュリティ・パフォーマンスなど最低限必須なもの
汚いが動く、とりあえずユーザに価値は届けられる
仕様を満たしている
バグってない
レビュー後のコメントについて
最近「LGTM」しか言ってない
ここを見ました、ここは見てません、みたいなことをちゃんと言うべきかもmrsekut.icon
レビューをお願いする時に考えるべきこと
開発に着手する前に合意を取って手戻りを減らす
rebaseなどしてcommitを整形するか
どういうcommit粒度にすべきか
fixupとかを駆使してキレイにしていく
どういうPR粒度にすべきか
例えば、リファクタ・フォーマットだけで1回PRを出す
PRには何を書くべきか
レビューしてもらう側
こういうノリあるんだ、なるほど
社内プロダクト開発においてはpull requestを出した当人がマージボタンを押す ref これは意見があると思うが個人的にはこだわりのスタイル
「自分が押したマージが本番に出ていく」という体験をしてもらう
コードベースへの当人のオーナーシップの醸成
というか、GithubのレビューページでVSCode並のシンタックスハイライトや、コンパイルエラー、unusedの表示などをしてくれればよいのだが。
レビュアーの自動割当
レビューする時
主軸が誰にあるのかという話がある
自分が完全にマネージする場合は、かなり強めに必要なルールの徹底をお願いする
そうでない場合は基本提案になりそう
書いている人は理解しているので、コメントの粒度の判断が難しい 記憶を忘れたときに自分でコメントを付ける必要がある
レビュワーが読んだときに、「ん？あ〜なるほど」となった箇所にちゃんとコメントを残すようにする
引っかかりポイントを減らす
今のチームは人数が少なすぎて、レビューの文化が薄いのであまり知見が溜まっていないmrsekut.icon
5つほどプロダクトがあって、エンジニアが2人なので、プロダクトごとに専属みたいになっている
だから、記事を読んだりして知見を溜めるしか無い
もっとOSSにcontributeしまくればいいかもしれない
でも、レビュワー側で参加することってあまりない
以下に書いてるのは、どこかの記事に書いてたやつで、良さそう、と思ったものをただ列挙したものmrsekut.icon
良さそう、と思っただけで実践しているわけではないmrsekut.icon
レビューはできるだけ早く行う
特に日を跨いだり、手戻りがあるとだるい
参考になるリンクを張る
[must]
必ず対応してほしい
[imo]
自分の意見や提案・好み。 自分ならこう書く
[nits]
些細な指摘
インデントやタイポ
[ask]
質問、確認。
コードの意味や背景が分からないから教えて
レビューで指摘→直す→再レビューで新しい指摘、をできるだけ避ける
レビューされる側はだるく感じだろうな、というのはわかるmrsekut.icon
「1回で言えよ」と思うと思う
しかし、コードの実装場所がおかしいことに対するレビューと、仕様的な問題に対するレビューはわけないと話がしづらい
最高2回ぐらいかな、と思っておけばいいだろうかmrsekut.icon
あるいは1回目のレビューをする時点で、
「たぶん修正後にもう一度修正があるかもしれません。なぜなら、」と伝えておくとか
伝え方の問題
すぐに正答例を示す
「自分で考えろ」からのダメ出しは心理的安全性が低いので避ける
「hogehogeはpiyoなので、良くない気がします」
具体例を提示する
こちらの方が良いと思う理由も添える
「私ならこうします」構文をつかう
アドバイスはするが、それに従わなくて良いというスタンス
テキストで指摘する
口頭だと、聞き取り手のメモが大変
良い感じの大きさにまとめたScpraboxを投げるのも良さそう
レビューの使い回しができる
レビューが終わったら、可能なら直接顔をあわせて言葉を交わす
テキストだけだと冷たい雰囲気になるので
なるほどmrsekut.icon
x あなたのメソッドの書き方は分かりにくいですよ。
o このメソッドは私には少々分かりにくいです。この変数にもっと良い名前をつけられないでしょうか。
こういうのも地味に大事なんだろうなmrsekut.icon
褒める
絵文字を使う
「これ読んで勉強し直してきて」はアンチパターン
上手くモチベートさせないといけないmrsekut.icon
「この本が参考になりました」とかは良いかも
でも別にレビュー時に言うことでもない気がする
別の機会の勉強会でそれとなく伝えるとか
本の内容がより頭に入るようになる
diffに現れない箇所も見る
バグを生みやすい箇所を重点的に見る
境界値検査とか
layer的に重要な箇所を重点的に見る
Entityとか、table構造とか
後から修正するのが大変な部分をしっかり見る
お金に関する処理とか
セキュリティ的に大丈夫か
法的に大丈夫か
UI、UXのチェック
初心者(?)にもレビューしてもらう
より詳しい人のコードを読んでもらう機会の創出になるので、プロダクトについての知識が増えるので良さそうではあるmrsekut.icon
意外と、レビュアーのコードがクソなことも多く、若手のメンバーのレビューはしっかり見てくれるため予期しない不具合を見つけてくれることも多く、実際的に有益です ref 例えば指摘項目がめちゃくちゃ多いときはどうするか
全部言う？小出しにする？
これは、たぶんそもそも事前に認識合わせをした方がいい気がするmrsekut.icon
あまりにも共有できていないと感じた時
実装前に方針を話しあう
PR数を増やす
google
新卒に向けた資料
入ったメンバーがコードを理解するためのコードレビューだったり、
パフォーマンスならベンチマーク取るとか、
コードレビュー時に設計の問題が出てくるのはあまりよくなくて、実装前に共有しておくべき、
みたいな話がある
文化が共有されてないと辛そう
なんでいちいち指摘するの？時間かかるだけじゃん、とか言われると辛い
レビューは、技術的な好みを語る場ではない、という主張
はてぶがおおい
PRに100件もコメントが付くようなやつを、以下に短縮していくかの工夫を色々